refactor(config-service): move site_settings write API from texera-web to config-service#6116
Open
Yicong-Huang wants to merge 6 commits into
Open
Conversation
…b to config-service
Fold AdminSettingsResource's GET/PUT/reset endpoints into ConfigResource
as /config/settings/{key}, so the service that seeds site_settings also
owns its read/write API. PUT and reset stay ADMIN-only; the single-key
GET keeps REGULAR+ADMIN because non-admin pages (dashboard logo/tabs,
dataset upload limits) read it. The frontend service now rides the
existing /api/config routing, so no proxy or nginx changes are needed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
engine
frontend
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6116 +/- ##
============================================
+ Coverage 59.11% 59.20% +0.09%
- Complexity 3201 3216 +15
============================================
Files 1132 1131 -1
Lines 43681 43706 +25
Branches 4734 4735 +1
============================================
+ Hits 25821 25878 +57
+ Misses 16430 16391 -39
- Partials 1430 1437 +7
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ead of string-based DSL Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 412 | 0.252 | 22,618/34,135/34,135 us | 🔴 -6.6% / 🔴 +125.4% |
| 🔴 | bs=100 sw=10 sl=64 | 938 | 0.573 | 103,083/139,280/139,280 us | 🔴 +10.0% / 🔴 +29.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,096 | 0.669 | 913,535/924,399/924,399 us | ⚪ within ±5% / 🟢 -12.8% |
Baseline details
Latest main 5c4a963 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 412 tuples/sec | 441 tuples/sec | 776.36 tuples/sec | -6.6% | -46.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.252 MB/s | 0.269 MB/s | 0.474 MB/s | -6.3% | -46.8% |
| bs=10 sw=10 sl=64 | p50 | 22,618 us | 23,120 us | 12,636 us | -2.2% | +79.0% |
| bs=10 sw=10 sl=64 | p95 | 34,135 us | 35,458 us | 15,143 us | -3.7% | +125.4% |
| bs=10 sw=10 sl=64 | p99 | 34,135 us | 35,458 us | 18,954 us | -3.7% | +80.1% |
| bs=100 sw=10 sl=64 | throughput | 938 tuples/sec | 991 tuples/sec | 985.33 tuples/sec | -5.3% | -4.8% |
| bs=100 sw=10 sl=64 | MB/s | 0.573 MB/s | 0.605 MB/s | 0.601 MB/s | -5.3% | -4.7% |
| bs=100 sw=10 sl=64 | p50 | 103,083 us | 97,492 us | 101,671 us | +5.7% | +1.4% |
| bs=100 sw=10 sl=64 | p95 | 139,280 us | 126,571 us | 107,939 us | +10.0% | +29.0% |
| bs=100 sw=10 sl=64 | p99 | 139,280 us | 126,571 us | 113,798 us | +10.0% | +22.4% |
| bs=1000 sw=10 sl=64 | throughput | 1,096 tuples/sec | 1,118 tuples/sec | 1,016 tuples/sec | -2.0% | +7.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.669 MB/s | 0.682 MB/s | 0.62 MB/s | -1.9% | +7.8% |
| bs=1000 sw=10 sl=64 | p50 | 913,535 us | 896,818 us | 989,693 us | +1.9% | -7.7% |
| bs=1000 sw=10 sl=64 | p95 | 924,399 us | 921,294 us | 1,028,327 us | +0.3% | -10.1% |
| bs=1000 sw=10 sl=64 | p99 | 924,399 us | 921,294 us | 1,059,969 us | +0.3% | -12.8% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,484.99,200,128000,412,0.252,22618.17,34135.45,34135.45
1,100,10,64,20,2131.30,2000,1280000,938,0.573,103083.02,139280.01,139280.01
2,1000,10,64,20,18240.78,20000,12800000,1096,0.669,913534.97,924399.32,924399.32…dded Postgres Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
dependencies
ConfigService main code uses SqlServer and the generated jOOQ tables directly but only got DAO transitively through Auth; declare it like the other services that touch the database do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dmin single-key GET Non-admin pages only need 19 whitelisted keys (branding, sidebar tabs, upload limits); serve those in one payload via GET /config/settings/public (REGULAR+ADMIN) and make the arbitrary single-key GET ADMIN-only. The frontend reads public settings through one cached request instead of a request per key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
xuang7
approved these changes
Jul 5, 2026
xuang7
left a comment
xuang7
left a comment
Contributor
There was a problem hiding this comment.
Generally LGTM! I left one comment.
Comment on lines
+47
to
+51
| getPublicSetting(key: string): Observable<string> { | ||
| if (!this.publicSettings$) { | ||
| this.publicSettings$ = this.http.get<Record<string, string>>(`${this.BASE_URL}/public`).pipe(shareReplay(1)); | ||
| } | ||
| return this.publicSettings$.pipe(map(settings => settings[key] ?? null)); |
Contributor
There was a problem hiding this comment.
getPublicSetting does not currently handle errors, and shareReplay(1) may cache the error. If the first /settings/public fetch fails, the error could be replayed to every consumer with no retry.
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in 0cac2ad: catchError now drops the cached observable and rethrows before shareReplay, so an errored fetch is never replayed and the next getPublicSetting call retries the request. Consumers keep their defaults on error, same as before.
…etch Address review: if the first /config/settings/public request errors, shareReplay(1) would replay that error to every consumer with no retry. Drop the cached observable on error so the next read retries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
AdminSettingsResource(GET/PUT/reset over thesite_settingstable) was registered in texera-web, while config-service already owns seeding of the same table at startup and has the JWT auth stack registered. This PR makes config-service the single owner of the table's API by folding the endpoints intoConfigResourceas the write side of the config API:GET /config/settings/public—REGULAR+ADMIN: the 19 whitelisted user-visible keys (dashboard branding, sidebar tab toggles, dataset upload limits) in one payload, replacing the request-per-key pattern (the dashboard alone fired 15).GET /config/settings/{key}—ADMINonly: management read over any key, used by the admin settings page.PUT /config/settings/{key}—ADMINonly (same as before)POST /config/settings/reset/{key}—ADMINonly (same as before)These are complementary to the existing HOCON-backed reads (
/config/pre-login,/config/gui,/config/amber,/config/user-system): those serve deploy-time config resolved from classpath files + env vars, frozen per JVM;/config/settings/*serves the DB-backed values that admins edit at runtime. There is no key overlap between the two families.Behavior change (intended tightening): the old texera-web GET had no role annotation, so anonymous and
RESTRICTED(pending-approval) users could read settings. Now the public read requires aREGULAR/ADMINlogin, matching every other/config/*endpoint (RESTRICTEDusers already get 403 on/config/gui), and arbitrary-key reads requireADMIN. The dashboard degrades gracefully for rejected users — the logo/tabs subscriptions simply keep their defaults.The queries use the generated jOOQ
SITE_SETTINGStable instead of the old string-basedDSL.table/DSL.fieldlookups, andConfigServicenow declares its directDAOdependency explicitly (it previously leaked in transitively throughAuth). A smallConfigSettingPojowire DTO keeps the JSON contract at exactly{key, value}— the generated pojo would also exposeupdated_by/updated_at.AdminSettingsResourceis deleted from texera-web. On the frontend,AdminSettingsServicemoves from/api/admin/settingsto/api/config/settings(riding the existing/api/configrouting in bothproxy.config.jsonand the single-node nginx config — no proxy/nginx/k8s changes needed) and gainsgetPublicSetting(key), which reads from one shared, cached/settings/publicrequest; the non-admin consumers (dashboard, files-uploader, dataset-detail) use it, while the admin settings page keeps the per-key ADMIN read. The admin page reloads the window after saving, so the cache never serves stale values.Any related issues, documentation, discussions?
Closes #6115
How was this PR tested?
sbt ConfigService/test: 27 tests pass.ConfigResourceAuthSpecpin the auth gates (anonymous → 401 with Bearer challenge on public/single-key/PUT; REGULAR single-key GET / PUT / reset → 403; ADMIN reset of a key with no default → 404, proving the role gate admits ADMIN). The spec now also registersAuthValueFactoryProvider.Binder, mirroring productionAuthFeatures.register, so@Auth SessionUserparameters resolve.ConfigSettingsCrudSpeccover the positive paths against an embedded Postgres (MockTexeraDB, via a newDAO % "test->test"dependency): read-miss → null, insert withupdated_byrecorded, upsert-on-conflict updates in place, null-value no-op, public map serves whitelisted keys and hides management-only ones, reset restores thedefault.confvalue, reset of an unknown key → 404.sbt WorkflowExecutionService/compilepasses after removing the resource from texera-web;sbt scalafmtAllclean.tsc --noEmitclean;ng testondashboard.component.spec.ts(11 tests) passes with the mock switched togetPublicSetting.bin/local-dev.sh, all 14 services), through the frontend dev proxy (:4200 → /api/config/** → config-service:9094):GET /settings/public→ 200 with the full whitelisted map; single-key GET → 403; PUT → 403;updated_byrecords the admin's username;/api/admin/settings/{key}now 404s.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-fable-5)